-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Revert node creation logic #1818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dtaniwaki, I readded TemplateScope
. Do you have any comments regarding the rest of the implementation? Is there a reason as to why you moved node creation before execute*
that I should be aware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simster7 Thank you for fixing the problem. The reason why I moved the initialization code to executeTemplate
is that I didn't want to pass orgTmpl
to each concrete executions and wanted to make sure a given node exists in the concrete execution. However, I now realized the behavior of some of the concrete execution is different from the others as you fixed here.
Is it possible to make some unit tests to avoid future regressions? If it's too hard, we can make it in a future task.
Will wait for #1836 to be merged before merging this |
@simster7 Could you use a concrete template over |
It would be great to get this moving again if possible so we can upgrade without hitting our resource limits! Thanks :) |
@benabineri We're planning on releasing this on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditional approval - please check that this is covered by some tests?
6f24261
to
8de2384
Compare
Codecov Report
@@ Coverage Diff @@
## master #1818 +/- ##
==========================================
+ Coverage 11.14% 11.15% +<.01%
==========================================
Files 35 35
Lines 23536 23537 +1
==========================================
+ Hits 2624 2625 +1
+ Misses 20576 20575 -1
- Partials 336 337 +1
Continue to review full report at Codecov.
|
func (woc *wfOperationCtx) executeDAG(nodeName string, tmplCtx *templateresolution.Context, templateScope string, tmpl *wfv1.Template, orgTmpl wfv1.TemplateHolder, boundaryID string) (*wfv1.NodeStatus, error) { | ||
node := woc.getNodeByName(nodeName) | ||
if node == nil { | ||
node = woc.initializeExecutableNode(nodeName, wfv1.NodeTypeSteps, templateScope, tmpl, orgTmpl, boundaryID, wfv1.NodeRunning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node type is not correct here.
I fixed it in this commit.
396faa0
Revert node creation back to
execute*
methods (as it was before #1552). The code flow should be the same as it is now, however this change allows individualexecute*
methods to control returning after a node is created once.In particular,
execute{Container, Script, Resource}
now return after a node has already been created in order to avoid callingcreateWorkflowPod
more than once. This should fix #1720.@dtaniwaki Since this code is mostly from your changes please review.